Repository navigation
Conversation
|
#6975 was recently merged. This PR should be rebased and adjusted. I will wait for that before reviewing. |
38a09b0 to
09199c4
Compare
hjmjohnson
left a comment
There was a problem hiding this comment.
Tested end to end in 3D Slicer against this PR's head (09199c4) on macOS: Slicer calls SetAutoloadPath(<Slicer_ITKFACTORIES_DIR>) followed by ReHash() in its application init, the launcher stops exporting ITK_AUTOLOAD_PATH, and the MRMLIDImageIO plugin loads in-process. The Slicer side, gated on a compile probe so ITK 5.4.x keeps the environment-variable path, is at hjmjohnson/Slicer#1. The implementation needs no change for that use; the inline notes are documentation and migration items.
The rebase onto #6975 that @dzenanz asked for is in place (the branch contains 4a643d8 and the tests sit inside the ITK_USE_AUTOLOAD_PATH block), so this looks ready to come out of draft.
Two more items without a natural inline anchor
- The PR body says Python exposure is unverified.
ObjectFactoryBaseis inITKCommonBase.wrap, so the two statics should appear, but wrapped builds haveITK_USE_AUTOLOAD_PATHoff, which makes the setter a stored no-op there. One sentence in the body would save a Python user from expecting it to load plugins. - Slicer needed
ReHash()because ITK's factory list is already initialized by the time the application's init runs;SetAutoloadPath()alone only stores the string. The header already says so, and it held in practice.
|
@blowekamp I created an initial set of slicer builds against this PR and asked for an extensive code review with various compile options to find corner-case failure modes. I think this PR is a great step forward, and I'd approve it as is, but the overnight builds and explorations identified a few items that should at least be noted in an ISSUE for future consideration. See the potential slicer changes at hjmjohnson/Slicer#1. |
09199c4 to
811244c
Compare
Initialize the autoload path from the environment once, when the ObjectFactoryBase globals are created, and expose ObjectFactoryBase::SetAutoloadPath()/GetAutoloadPath() so a host can set it at runtime before the factories are first loaded. ReHash() uses the stored path instead of re-reading the environment. Add standalone tests for setting the path by environment and by API, and use the setter in itkObjectFactoryTest2. The tests use only CMake and ITK facilities available on release-5.4 so the change can be backported; the CMake guard needs adapting there. Relates to InsightSoftwareConsortium#6982 and InsightSoftwareConsortium#6975. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
811244c to
7ec3021
Compare
ReHash() no longer re-reads ITK_AUTOLOAD_PATH; callers changing the path after startup must use ObjectFactoryBase::SetAutoloadPath(). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
7ec3021 to
32419f3
Compare
dzenanz
left a comment
There was a problem hiding this comment.
I have not taken a good look at the tests. The rest looks good on a glance.
Store the
ITK_AUTOLOAD_PATHvalue in theObjectFactoryBaseglobals and addObjectFactoryBase::SetAutoloadPath()/GetAutoloadPath(), so the factory search path can be configured at runtime, before the factories are dynamically loaded.Related: #6982 (decouple the autoload scan from
ITK_WRAPPING); builds on #6975 (merged), which introducedITK_USE_AUTOLOAD_PATH.Change
ITK_AUTOLOAD_PATHenvironment variable when theObjectFactoryBaseglobals are created, and is held there instead of being read from the environment insideLoadDynamicFactories().SetAutoloadPath(const std::string &)andGetAutoloadPath(). The setter takes effect if called before the firstInitialize()/CreateInstance(), or followed byReHash().ReHash()now uses the stored path rather than re-reading the environment. Code that changesITK_AUTOLOAD_PATHwithputenvafter startup and then callsReHash()must callSetAutoloadPath()instead;itkObjectFactoryTest2is updated accordingly.itkObjectFactoryAutoloadPathTest, run two ways:itkObjectFactoryAutoloadPathEnvTest(CTest setsITK_AUTOLOAD_PATH, nothing configured inmain()) anditkObjectFactoryAutoloadPathApiTest(variable unset,SetAutoloadPath()called inmain()). Both check thatFactoryTestLibis loaded on first use and not loaded after the path is cleared andReHash()is called.How this relates to #6982 and #6975
#6982 asks to replace the compile-time
NOT ITK_WRAPPINGrule with a control that lives where the hazard is: two ITK builds sharing one process, e.g. an ITK wheel imported inside 3D Slicer whoseITK_AUTOLOAD_PATHnames plugins built against Slicer's ITK. This PR does not change when the scan happens. It supplies a runtime control that those options can build on:SetAutoloadPath("")(or its own plugin directory) before ITK is first used, instead of relying on the process environment.ObjectFactoryBaseis already wrapped, so the statics should also be reachable from Python (not yet verified in a wrapped build). In wrapped buildsITK_USE_AUTOLOAD_PATHis off, so the setter only stores the value and no plugins are loaded. It does not require clearing or renaming the environment variable, which other tools in the process may read.For #6975 (merged), the plugin tests it guards with
ITK_USE_AUTOLOAD_PATHrely onITK_AUTOLOAD_PATHbeing read by the scan. With a setter, tests no longer need to mutate the process environment to point at a plugin directory.The new tests are registered inside the
ITK_BUILD_SHARED_LIBS AND ITK_USE_AUTOLOAD_PATHblock from #6975, so they are skipped in builds whereObjectFactoryBasedoes not scan the autoload path (e.g. Python-wrapped builds).Backport: the ENH commit is self-contained and avoids CMake features newer than 3.16.3 (the api test unsets the variable in-process rather than using
ENVIRONMENT_MODIFICATION), so it can be cherry-picked torelease-5.4; the test guard needs adapting there sinceITK_USE_AUTOLOAD_PATHis not on that branch. The ITK 6 migration note is a separate DOC commit that stays on main.Local verification (Linux x86_64, gcc)
Rebased onto main including #6975. Shared build (
BUILD_SHARED_LIBS=ON,ITKCommononly),ctest -R ObjectFactory:itkObjectFactoryAutoloadPathEnvTest: passeditkObjectFactoryAutoloadPathApiTest: passeditkObjectFactoryTest2: passeditkObjectFactoryTest3: passeditkObjectFactoryOnlyNewTest: passedTemporary logging confirmed the order in both modes: globals created, then (API mode)
SetAutoloadPath(), then the firstInitialize()/LoadDynamicFactories()with the configured path. Not run:itkObjectFactoryTest(target not built), static build with the final change, Python-wrapped build, other platforms.AI assistance
Implemented with an AI coding assistant (Copilot) under the author's direction.
PR Checklist
ReHash()no longer re-reads the environment) is documented in the header and the ITK 6 migration guideObjectFactoryBase